-
Notifications
You must be signed in to change notification settings - Fork 260
Badriev A. (Basic_exercises) #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
krepysh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В целом здорово: несколько замечаний ко всему коду:
- Не используй lambda функции без острой необходимости (в этих задачах ее нет, избавься от всех лямбд)
- Лучше разделяй код: return вместо print, выноси инпут наружу, разделяй сложные функции на функции попроще. Где возможно переиспользуй фунции: использую решение предыдущей задачи как подпрограмму.
- Наименование функции должно отражать что она возвращает.
for_challenges.py
Outdated
| ] | ||
| # ??? | ||
|
|
||
| group_token = lambda group_to: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не используй лямбда функции без острой необходимости
К тому же здесь ты даже ей имя даешь. Обычный def будет читаемее
https://towardsdatascience.com/the-python-lambda-function-has-become-a-devil-d0340404abcb
Переделай в обычные функции
for_challenges.py
Outdated
|
|
||
| student_token = lambda student_to: ( | ||
| (student_to in range(5, 20)) and 'учеников' or | ||
| (1 in (student_to, (diglast := student_to % 10))) and 'ученик' or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Классно что было интересно заняться этим, но это не подразумевалось в задании.
Доп. вопрос можно ли сделать это решение универсальнее, что бы не можно было использовать эту логику для почти любого существительного : "парта" "шкаф"?
for_challenges.py
Outdated
| ] | ||
| # ??? | ||
|
|
||
| for key, value in dict(enumerate(groups, start=1)).items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
вот тут не нужен dict().items()
просто for group, group_number in enumerate.
и постарайся запомнить порядок элементов в tuple от enumerate.
for_dict_challenges.py
Outdated
| # ??? | ||
| names = [] | ||
| for student in students: | ||
| names.append(student['first_name']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно ли переделать с использование list comprehension?
for_dict_challenges.py
Outdated
| print('_' * 75) | ||
|
|
||
| # print(pd.DataFrame(names).mode()[0][0]) | ||
| print(f'Самое частое имя среди учеников: {max(names, key=lambda x: names.count(x))}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Посчитай по честному, через числа в словаре
for_dict_challenges_bonus.py
Outdated
| } | ||
|
|
||
| for message in messages: | ||
| if float(message["sent_at"].strftime('%H.%M')) < 12: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это очень непрямой способ проверять время.
Судя по всему sent_at это datetime, просто обратись к атрибуту .hour
и вытащи это в отдельную переменную, что бы message["sent_at"] использовался только один раз в этом цикле, а в if проверяй эту переменную.
for_dict_challenges_bonus.py
Outdated
| def task5(messages): | ||
| result = [] | ||
|
|
||
| for k, g in groupby([message['reply_for'] for message in messages]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k, g - плохие имена
Назови так что бы было понятно что там лежит.
for_dict_challenges_bonus.py
Outdated
| result.append((k, length)) | ||
|
|
||
| lst = [(key, value) for key, value in sorted(result, key=lambda x: x[1], reverse=True) if | ||
| key is not None and value > 1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А зачем этот if?
for_dict_challenges_bonus.py
Outdated
|
|
||
| if len(lst) > 0: | ||
| for key, value in lst: | ||
| print(f'Сообщение под айди: "{key}" стало началом для самых длинных тредов (цепочек ответов), а именно ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return instead of print
| word = 'Архангельск' | ||
| # ??? | ||
|
|
||
| vowels = 'аеиоуэюяыё' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
krepysh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Значительно лучше, пара мест в которых можно поправить нейминг, и одно место в котором создаешь слишком много объектов set, а это относительно дорого по времени (и потенциально памяти)
| return 'групп' | ||
| elif 1 in (numb, (diglast := numb % 10)): | ||
| return 'группа' | ||
| elif {numb, diglast} & {2, 3, 4}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тот редкий случай когда строчка комментария не помешала бы.
for_dict_challenges.py
Outdated
| # Петя: 2 | ||
|
|
||
| # создадим функцию, которая возвращает список студентов по ключу 'first_name' | ||
| def student_list(lst): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
нэйминг надо поправить. пример: get_students_first_names(student_list):
То есть в идеале, название функции должно отражать тот смысл который ты передаешь комментарием.
for_dict_challenges.py
Outdated
| # ??? | ||
|
|
||
|
|
||
| def find_girls(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
нэйминг. название функции должно отражать, что ты возвращаешь подсчитанное число.
И передавай group явно, через аргументы.
Обращаться к какому-то объекту из внешней области видимости плохая практика. Это приводит к неочевидным ошибкам. В твоем случае функции перестанут работать если ты переименуешь переменную в цикле.
for_dict_challenges.py
Outdated
|
|
||
| for group in school: | ||
| if find_boys() > find_girls(): | ||
| print(f'Больше всего мальчиков в классе: {group["class"]}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут надо было найти класс, в котором наибольшее количество мальчиков, а не все классы в которых мальчиков больше чем девочек.
| return maximum_frequency(create_list_by_key('sent_by', messages)) | ||
|
|
||
|
|
||
| def find_id_who_got_the_most_replies(messages): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Классный нейминг!
for_dict_challenges_bonus.py
Outdated
| if message['sent_by'] not in id_unique: | ||
| id_unique[message['sent_by']] = set(message['seen_by']) | ||
| else: | ||
| id_unique[message['sent_by']] = set(id_unique[message['sent_by']]) | set(message['seen_by']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это кажется дорогая операция, много раз пересоздаешь сет.
| id_unique[message['sent_by']] = set(id_unique[message['sent_by']]) | set(message['seen_by']) | |
| id_unique[message['sent_by']] = id_unique[message['sent_by']].update(message['seen_by']) |
Если использовать id_unique.get(message['sent_by'], set()), то можно еще и от if избавиться.
for_dict_challenges_bonus.py
Outdated
| for message in messages: | ||
| hour_of_writing = message["sent_at"].hour | ||
| if hour_of_writing < 12: | ||
| times['morning'].append(hour_of_writing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
с таким же успехом тут может быть переменная список morning. Не вижу как словарь тут упрощает что-то.
No description provided.